Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for CPU and memory isolators #610

Merged
merged 5 commits into from
Dec 22, 2015
Merged

Add support for CPU and memory isolators #610

merged 5 commits into from
Dec 22, 2015

Conversation

achanda
Copy link
Contributor

@achanda achanda commented Dec 21, 2015

No description provided.

The rkt community added supprt for these isolators recently
@@ -149,6 +149,19 @@ func (d *RktDriver) Start(ctx *ExecContext, task *structs.Task) (DriverHandle, e
cmd_args = append(cmd_args, fmt.Sprintf("--exec=%v", exec_cmd))
}

if task.Resources.MemoryMB == 0 {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how do you say "infinite" or "don't care at all" ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jippi Drivers that support resource control, shouldn't allow infinite resources in my opinion since from the point of view of the scheduler finite resources were being assigned to a task.

@achanda
Copy link
Contributor Author

achanda commented Dec 21, 2015

@diptanu thanks, updated

@@ -133,33 +133,46 @@ func (d *RktDriver) Start(ctx *ExecContext, task *structs.Task) (DriverHandle, e
envVars.ClearAllocDir()

for k, v := range envVars.Map() {
cmd_args = append(cmd_args, fmt.Sprintf("--set-env=%v=%v", k, v))
cmdArgs = append(cmdArgs, fmt.Sprintf("--set-env=%v=%v", k, v))
}

// Disble signature verification if the trust command was not run.
if !trust_cmd {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this one should be camelCase too.

}

// Add memory isolator
cmdArgs = append(cmdArgs, fmt.Sprintf("--memory=%vM", int64(task.Resources.MemoryMB)*1024*1024))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IIRC this was not previously supported in rkt. Does this require a particular version of rkt? If so we should check in the fingerprinter that we have a version that supports this.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, this was added in 0.14.0, rkt/rkt#1851
I assumed we were targeting to converge after post 1.0, so that it's okay to not be backwards compatible for now. If we think the check is necessary, I can add it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's fine not to be BC here but we should still check that the version is supported so we can tell the user to upgrade instead of failing later when a task is started.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cbednarski @achanda I almost think we should not make the rkt driver available if the version is less than 0.14

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@diptanu I agree, in favor of uniformity across all the drivers.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Move 1024*1024 to a constant at the top of the file

@@ -85,6 +86,13 @@ func (d *RktDriver) Fingerprint(cfg *config.Config, node *structs.Node) (bool, e
node.Attributes["driver.rkt.version"] = rktMatches[1]
node.Attributes["driver.rkt.appc.version"] = appcMatches[1]

minVersion, _ := version.NewVersion("0.14.0")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would also put this in a const/variable at the top and put a comment on why we only support 0.14.0 and above.

@dadgar
Copy link
Contributor

dadgar commented Dec 22, 2015

@achanda Thanks!

dadgar added a commit that referenced this pull request Dec 22, 2015
Add support for CPU and memory isolators
@dadgar dadgar merged commit 8ad5fd2 into hashicorp:master Dec 22, 2015
@achanda achanda deleted the isolators branch December 22, 2015 18:13
@github-actions
Copy link

I'm going to lock this pull request because it has been closed for 120 days ⏳. This helps our maintainers find and focus on the active contributions.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 30, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants